Minor: Ctrl+C Termination in CLI #8739
Conversation
berkaysynnada
changed the title
There was a problem hiding this comment.
Thanks @berkaysynnada
What will happen with already running queries/streams if Ctrl+C get pressed?
| Ok(_) => {} | ||
| Err(err) => eprintln!("{err}"), | ||
| tokio::select! { | ||
| res = exec_and_print(ctx, print_options, line) => match res { |
There was a problem hiding this comment.
Looks like CTRL+C just getting out of the loop without actual query interruption?
So the machine resources is not released when the user tries to run next query after CTRL+C?
There was a problem hiding this comment.
My understanding was that when control arrives there, tasks were already interrupted. But I could be wrong -- in that case this PR needs to address your point
There was a problem hiding this comment.
I think this code is correct.
My understanding of what happens is that if the signal is handled, it will drop the future running exec_and_print which will implicitly is droped which should then propagate back and cancel outstanding tasks
There are a variety of tests that validate that outstanding tasks are cancelled when their containing streams are dropped such as https://github.com/apache/arrow-datafusion/blob/1179a76567892b259c88f08243ee01f05c4c3d5c/datafusion/physical-plan/src/coalesce_partitions.rs#L216-L234
If we find instances where dropping a stream doesn't cancel the ongoing execution, I think we should file that as a bug and fix it
There was a problem hiding this comment.
🤔 it might be good to add some documentation to the https://docs.rs/datafusion/latest/datafusion/physical_plan/trait.ExecutionPlan.html#tymethod.execute to explain the drop / cancel behavior and expectations this better. I will add that to my list
There was a problem hiding this comment.
I think this is correct, too
Per tokio::select documentation:
Waits on multiple concurrent branches, returning when the first branch completes, cancelling the remaining branches.
This is also what I tried initially: #1279 (comment)
But I had the roadblock of exec_and_print being blocking at that time. That doesn't seem to be the case anymore so this should be good.
There was a problem hiding this comment.
Here is a proposed PR with some clarifying documentation : #8747
There was a problem hiding this comment.
But I had the roadblock of exec_and_print being blocking at that time. That doesn't seem to be the case anymore so this should be good.
I think @berkaysynnada / @ozankabak made the output mostly streaming in #8651
| Ok(_) => {} | ||
| Err(err) => eprintln!("{err}"), | ||
| tokio::select! { | ||
| res = exec_and_print(ctx, print_options, line) => match res { |
There was a problem hiding this comment.
I think this code is correct.
My understanding of what happens is that if the signal is handled, it will drop the future running exec_and_print which will implicitly is droped which should then propagate back and cancel outstanding tasks
There are a variety of tests that validate that outstanding tasks are cancelled when their containing streams are dropped such as https://github.com/apache/arrow-datafusion/blob/1179a76567892b259c88f08243ee01f05c4c3d5c/datafusion/physical-plan/src/coalesce_partitions.rs#L216-L234
If we find instances where dropping a stream doesn't cancel the ongoing execution, I think we should file that as a bug and fix it
| Ok(_) => {} | ||
| Err(err) => eprintln!("{err}"), | ||
| tokio::select! { | ||
| res = exec_and_print(ctx, print_options, line) => match res { |
There was a problem hiding this comment.
🤔 it might be good to add some documentation to the https://docs.rs/datafusion/latest/datafusion/physical_plan/trait.ExecutionPlan.html#tymethod.execute to explain the drop / cancel behavior and expectations this better. I will add that to my list
|
Relates to this issue I believe? |
|
Thanks for the reviews everyone! |
Which issue does this PR close?
Closes #.
Rationale for this change
For streams and long running queries, we will be able to abort the execution and return back to the
readline()state without terminating the CLI session with ctrl+c signal.What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?